Re: [Xen-devel] [PATCH v4 2/5] xen/arm: vgic-v2: Don't ignore a write in ITARGETSR if one field is 0

2015-10-23 Thread Ian Campbell 
On Thu, 2015-10-22 at 17:51 +0100, Julien Grall wrote:
> On 22/10/15 17:07, Ian Campbell wrote:
> > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > > index 665afeb..6b7eab3 100644
> > > --- a/xen/arch/arm/vgic-v2.c
> > > +++ b/xen/arch/arm/vgic-v2.c
> > > @@ -50,6 +50,94 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t
> > > cbase,
> > > paddr_t vbase)
> > >  vgic_v2_hw.vbase = vbase;
> > >  }
> > >  
> > > +#define NR_TARGET_PER_REG 4U
> > > +#define NR_BIT_PER_TARGET 8U
> > 
> > NR_TARGETS_ and NR_BITS_...
> > 
> > "REG" there is a bit generic, given this only works for registers with
> > 8
> > -bit fields, _ITARGETSR instead?
> 
> Well this is within the vgic-v2.c and only one register contains target.
> So I found pointless to add ITARGETSR to the name.

It's the use of the generic "REG" when there is only one relevant register
(which could be named) which I found confusing, since the current name
implies it has wider relevance than it actually does.

> > This is really a part of the for() iteration expression, but oddly
> > place
> > here.
> > If you turn the "((1 << NR_BIT_PER_TARGET) - 1)" thing into a #define
> > or
> > constant, then you may find that extracting the relevant byte from the
> > unshifted itargetsr using (itargetsr >> (i*NR_BITS_PER_TARGET) and then
> > applying the mask is clean enough to use here instead.
> 
> I placed this shift here because I didn't want to use ... >> (i *
> NR_BIT_..) which require a multiplication rather than a shift in the
> resulting code.

FWIW given a constant NR_BITS which is a power of two I think i*NR_BITS
would end up a shift with any reasonable compiler.

> > > + * guest).
> > > + */
> > > +if ( !new_target || (new_target > d->max_vcpus) )
> > > +{
> > > +printk(XENLOG_G_DEBUG
> > 
> > gdprintk?
> 
> I would prefer to keep this printk in non-debug to help us catching any
> OS potentially using this trick.
> 
> Based on that I would even use XENLOG_G_WARNING because this is not
> really compliant to the spec and we are meant to fix it.

Assuming I remember correctly that XENLOG_G_WARNING is ratelimited in
default configurations, then OK.

> 
> Regards,
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 2/5] xen/arm: vgic-v2: Don't ignore a write in ITARGETSR if one field is 0

2015-10-23 Thread Julien Grall 
Hi Ian,

On 23/10/15 10:30, Ian Campbell wrote:
> On Thu, 2015-10-22 at 17:51 +0100, Julien Grall wrote:
>> On 22/10/15 17:07, Ian Campbell wrote:
 diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
 index 665afeb..6b7eab3 100644
 --- a/xen/arch/arm/vgic-v2.c
 +++ b/xen/arch/arm/vgic-v2.c
 @@ -50,6 +50,94 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t
 cbase,
 paddr_t vbase)
  vgic_v2_hw.vbase = vbase;
  }
  
 +#define NR_TARGET_PER_REG 4U
 +#define NR_BIT_PER_TARGET 8U
>>>
>>> NR_TARGETS_ and NR_BITS_...
>>>
>>> "REG" there is a bit generic, given this only works for registers with
>>> 8
>>> -bit fields, _ITARGETSR instead?
>>
>> Well this is within the vgic-v2.c and only one register contains target.
>> So I found pointless to add ITARGETSR to the name.
> 
> It's the use of the generic "REG" when there is only one relevant register
> (which could be named) which I found confusing, since the current name
> implies it has wider relevance than it actually does.

Ok. I guess you'd like to rename NR_BITS_PER_TARGET? If so, do you have
any suggestion?

>>> This is really a part of the for() iteration expression, but oddly
>>> place
>>> here.
>>> If you turn the "((1 << NR_BIT_PER_TARGET) - 1)" thing into a #define
>>> or
>>> constant, then you may find that extracting the relevant byte from the
>>> unshifted itargetsr using (itargetsr >> (i*NR_BITS_PER_TARGET) and then
>>> applying the mask is clean enough to use here instead.
>>
>> I placed this shift here because I didn't want to use ... >> (i *
>> NR_BIT_..) which require a multiplication rather than a shift in the
>> resulting code.
> 
> FWIW given a constant NR_BITS which is a power of two I think i*NR_BITS
> would end up a shift with any reasonable compiler.

I will give a look.

> 
 + * guest).
 + */
 +if ( !new_target || (new_target > d->max_vcpus) )
 +{
 +printk(XENLOG_G_DEBUG
>>>
>>> gdprintk?
>>
>> I would prefer to keep this printk in non-debug to help us catching any
>> OS potentially using this trick.
>>
>> Based on that I would even use XENLOG_G_WARNING because this is not
>> really compliant to the spec and we are meant to fix it.
> 
> Assuming I remember correctly that XENLOG_G_WARNING is ratelimited in
> default configurations, then OK.

Correct, any XENLOG_G_* is rate-limited by default.

Regards,

-- 
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 2/5] xen/arm: vgic-v2: Don't ignore a write in ITARGETSR if one field is 0

2015-10-23 Thread Ian Campbell 
On Fri, 2015-10-23 at 10:37 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 23/10/15 10:30, Ian Campbell wrote:
> > On Thu, 2015-10-22 at 17:51 +0100, Julien Grall wrote:
> > > On 22/10/15 17:07, Ian Campbell wrote:
> > > > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > > > > index 665afeb..6b7eab3 100644
> > > > > --- a/xen/arch/arm/vgic-v2.c
> > > > > +++ b/xen/arch/arm/vgic-v2.c
> > > > > @@ -50,6 +50,94 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t
> > > > > cbase,
> > > > > paddr_t vbase)
> > > > >  vgic_v2_hw.vbase = vbase;
> > > > >  }
> > > > >  
> > > > > +#define NR_TARGET_PER_REG 4U
> > > > > +#define NR_BIT_PER_TARGET 8U
> > > > 
> > > > NR_TARGETS_ and NR_BITS_...
> > > > 
> > > > "REG" there is a bit generic, given this only works for registers
> > > > with
> > > > 8
> > > > -bit fields, _ITARGETSR instead?
> > > 
> > > Well this is within the vgic-v2.c and only one register contains
> > > target.
> > > So I found pointless to add ITARGETSR to the name.
> > 
> > It's the use of the generic "REG" when there is only one relevant
> > register
> > (which could be named) which I found confusing, since the current name
> > implies it has wider relevance than it actually does.
> 
> Ok. I guess you'd like to rename NR_BITS_PER_TARGET? If so, do you have
> any suggestion?

NR_BITS_PER_TARGET is OK here, since 8 is always correct for that in the
gic v2 irrespective of whether it is used with any register.

NR_TARGET_PER_REG was the thing I thought we were discussing above. I think
that one should be NR_TARGETS_PER_ITARGETSR.

Alternatively
const unsigned long nr_targets_per_reg = 32/NR_BITS_PER_TARGET;
in the context of the vgic_store_itargetsr function (and others which use
it) would be ok too. Or maybe 32=>(sizeof(...)*8). And maybe in that
context an even terser name would be ok too.

Ian.


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 2/5] xen/arm: vgic-v2: Don't ignore a write in ITARGETSR if one field is 0

2015-10-22 Thread Ian Campbell 
On Mon, 2015-10-12 at 15:22 +0100, Julien Grall wrote:
> The current implementation ignores the whole write if one of the field is
> 0. Although, based on the spec (4.3.12 IHI 0048B.b), 0 is a valid value
> when:
> - The interrupt is not wired in the distributor. From the Xen
> point of view, it means that the corresponding bit is not set in
> d->arch.vgic.allocated_irqs.
> - The user wants to disable the IRQ forwarding in the distributor.
> I.e the IRQ stays pending in the distributor and never received by
> the guest.
> 
> Implementing the later will require more work in Xen because we always
> assume the interrupt is forwarded to vCPU. So for now, ignore any field
> where the value is 0.
> 
> The emulation of the write access of ITARGETSR has been reworked and
> moved to a new function because it would have been difficult to
> implement properly the behavior with the current code.
> 
> The new implementation is breaking the register in 4 distinct bytes. For
> each byte, it will check the validity of the target list, find the new
> target, migrate the interrupt and store the value if necessary.
> 
> In the new implementation there is nearly no distinction of the access
> size to avoid having too many different path which is harder to test.
> 
> Signed-off-by: Julien Grall 
> 
> ---
> This change used to be embedded in "xen/arm: vgic: Optimize the way
> to store the target vCPU in the rank". It has been moved out to
> avoid having too much functional changes in a single patch.
> 
> I'm not sure if this patch should be backported to Xen 4.6. Without
> it any guest writing 0 in one the field won't be able to migrate
> other interrupts. Although, in all the use case I've seen, the guest
> is read ITARGETSR first and write-back the value with the
> corresponding byte changed.
> 
> Changes in v4:
> - Patch added
> ---
>  xen/arch/arm/vgic-v2.c | 141 ++-
> --
>  1 file changed, 98 insertions(+), 43 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 665afeb..6b7eab3 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -50,6 +50,94 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase,
> paddr_t vbase)
>  vgic_v2_hw.vbase = vbase;
>  }
>  
> +#define NR_TARGET_PER_REG 4U
> +#define NR_BIT_PER_TARGET 8U

NR_TARGETS_ and NR_BITS_...

"REG" there is a bit generic, given this only works for registers with 8
-bit fields, _ITARGETSR instead?

> +/*
> + * Store a ITARGETSR register. This function only deals with ITARGETSR8
> + * and onwards.
> + *
> + * Note the offset will be aligned to the appropriate boundary.
> + */
> +static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank
> *rank,
> + unsigned int offset, uint32_t
> itargetsr)
> +{
> +unsigned int i;
> +unsigned int regidx = REG_RANK_INDEX(8, offset, DABT_WORD);
> +unsigned int virq;
> +
> +ASSERT(spin_is_locked(>lock));
> +
> +/*
> + * The ITARGETSR0-7, used for SGIs/PPIs, are implemented RO in the
> + * emulation and should never call this function.
> + *
> + * They all live in the first rank.
> + */
> +BUILD_BUG_ON(NR_INTERRUPT_PER_RANK != 32);
> +ASSERT(rank->index >= 1);
> +
> +offset &= INTERRUPT_RANK_MASK;
> +offset &= ~(NR_TARGET_PER_REG - 1);
> +
> +virq = rank->index * NR_INTERRUPT_PER_RANK + offset;
> +
> +for ( i = 0; i < NR_TARGET_PER_REG; i++, offset++, virq++ )
> +{
> +unsigned int new_target, old_target;
> +uint8_t new_mask, old_mask;
> +
> +new_mask = itargetsr & ((1 << NR_BIT_PER_TARGET) - 1);
> +old_mask = vgic_byte_read(rank->v2.itargets[regidx], i);
> +
> +itargetsr >>= NR_BIT_PER_TARGET;

This is really a part of the for() iteration expression, but oddly place
here.

If you turn the "((1 << NR_BIT_PER_TARGET) - 1)" thing into a #define or
constant, then you may find that extracting the relevant byte from the
unshifted itargetsr using (itargetsr >> (i*NR_BITS_PER_TARGET) and then
applying the mask is clean enough to use here instead.

I don't think you rely on the shifting of itargetsr apart from when
calculating new_mask.

> +/*
> + * SPIs are using the 1-N model (see 1.4.3 in ARM IHI 0048B).
> + * While the interrupt could be set pending to all the vCPUs in
> + * target list, it's not guarantee by the spec.

"guaranteed"

> + * For simplicity, always route the vIRQ to the first interrupt
> + * in the target list
> + */
> +new_target = ffs(new_mask);
> +old_target = ffs(old_mask);
> +
> +/* The current target should always be valid */
> +ASSERT(old_target && (old_target <= d->max_vcpus));
> +
> +/*
> + * Ignore the write request for this interrupt if the new target
> + * is invalid.
>

Re: [Xen-devel] [PATCH v4 2/5] xen/arm: vgic-v2: Don't ignore a write in ITARGETSR if one field is 0

2015-10-22 Thread Julien Grall 
On 22/10/15 17:07, Ian Campbell wrote:
>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>> index 665afeb..6b7eab3 100644
>> --- a/xen/arch/arm/vgic-v2.c
>> +++ b/xen/arch/arm/vgic-v2.c
>> @@ -50,6 +50,94 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase,
>> paddr_t vbase)
>>  vgic_v2_hw.vbase = vbase;
>>  }
>>  
>> +#define NR_TARGET_PER_REG 4U
>> +#define NR_BIT_PER_TARGET 8U
> 
> NR_TARGETS_ and NR_BITS_...
> 
> "REG" there is a bit generic, given this only works for registers with 8
> -bit fields, _ITARGETSR instead?

Well this is within the vgic-v2.c and only one register contains target.
So I found pointless to add ITARGETSR to the name.

> 
>> +/*
>> + * Store a ITARGETSR register. This function only deals with ITARGETSR8
>> + * and onwards.
>> + *
>> + * Note the offset will be aligned to the appropriate boundary.
>> + */
>> +static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank
>> *rank,
>> + unsigned int offset, uint32_t
>> itargetsr)
>> +{
>> +unsigned int i;
>> +unsigned int regidx = REG_RANK_INDEX(8, offset, DABT_WORD);
>> +unsigned int virq;
>> +
>> +ASSERT(spin_is_locked(>lock));
>> +
>> +/*
>> + * The ITARGETSR0-7, used for SGIs/PPIs, are implemented RO in the
>> + * emulation and should never call this function.
>> + *
>> + * They all live in the first rank.
>> + */
>> +BUILD_BUG_ON(NR_INTERRUPT_PER_RANK != 32);
>> +ASSERT(rank->index >= 1);
>> +
>> +offset &= INTERRUPT_RANK_MASK;
>> +offset &= ~(NR_TARGET_PER_REG - 1);
>> +
>> +virq = rank->index * NR_INTERRUPT_PER_RANK + offset;
>> +
>> +for ( i = 0; i < NR_TARGET_PER_REG; i++, offset++, virq++ )
>> +{
>> +unsigned int new_target, old_target;
>> +uint8_t new_mask, old_mask;
>> +
>> +new_mask = itargetsr & ((1 << NR_BIT_PER_TARGET) - 1);
>> +old_mask = vgic_byte_read(rank->v2.itargets[regidx], i);
>> +
>> +itargetsr >>= NR_BIT_PER_TARGET;
> 
> This is really a part of the for() iteration expression, but oddly place
> here.
> If you turn the "((1 << NR_BIT_PER_TARGET) - 1)" thing into a #define or
> constant, then you may find that extracting the relevant byte from the
> unshifted itargetsr using (itargetsr >> (i*NR_BITS_PER_TARGET) and then
> applying the mask is clean enough to use here instead.

I placed this shift here because I didn't want to use ... >> (i *
NR_BIT_..) which require a multiplication rather than a shift in the
resulting code.

Furthermore, I found the for loop more complicate to read with itargetsr
>>= inside.

Anyway, I prefer the latter solution so I will use it in the next version.

> 
> I don't think you rely on the shifting of itargetsr apart from when
> calculating new_mask.

Correct.

>> +/*
>> + * SPIs are using the 1-N model (see 1.4.3 in ARM IHI 0048B).
>> + * While the interrupt could be set pending to all the vCPUs in
>> + * target list, it's not guarantee by the spec.
> 
> "guaranteed"
> 
>> + * For simplicity, always route the vIRQ to the first interrupt
>> + * in the target list
>> + */
>> +new_target = ffs(new_mask);
>> +old_target = ffs(old_mask);
>> +
>> +/* The current target should always be valid */
>> +ASSERT(old_target && (old_target <= d->max_vcpus));
>> +
>> +/*
>> + * Ignore the write request for this interrupt if the new target
>> + * is invalid.
>> + * XXX: From the spec, if the target list is not valid, the
>> + * interrupt should be ignored (i.e not forwarding to the
> 
> "forwarded".
> 
>> + * guest).
>> + */
>> +if ( !new_target || (new_target > d->max_vcpus) )
>> +{
>> +printk(XENLOG_G_DEBUG
> 
> gdprintk?

I would prefer to keep this printk in non-debug to help us catching any
OS potentially using this trick.

Based on that I would even use XENLOG_G_WARNING because this is not
really compliant to the spec and we are meant to fix it.

Regards,

-- 
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel