>>> On 16.04.15 at 12:53, <t...@xen.org> wrote:
> At 14:17 +0100 on 14 Apr (1429021061), Andrew Cooper wrote:
>> On 14/04/15 12:47, Jan Beulich wrote:
>> >>>> On 13.04.15 at 18:01, <dsl...@verizon.com> wrote:
>> >> --- a/xen/arch/x86/hvm/hvm.c
>> >> +++ b/xen/arch/x86/hvm/hvm.c
>> >> @@ -536,8 +536,9 @@ static int hvm_alloc_ioreq_gmfn(struct domain *d,
>> >> unsigned long *gmfn)
>> >>
>> >> static void hvm_free_ioreq_gmfn(struct domain *d, unsigned long gmfn)
>> >> {
>> >> - unsigned int i = gmfn - d->arch.hvm_domain.ioreq_gmfn.base;
>> >> + unsigned long i = gmfn - d->arch.hvm_domain.ioreq_gmfn.base;
>> >>
>> >> + BUG_ON(i >= sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8);
>> >> clear_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
>> >> }
>> > I'd be happier with an ASSERT() - Andrew?
>>
>> If I recall, this is a follow on from the compiler error, where gmfn now
>> gets initialised to ~0 to avoid a build failure.
>>
>> If gcc is correct and there is a way for gmfn to be used, then the
>> clear_bit() here clobber memory. The BUG_ON() serves as a protection
>> against the clobbering.
>>
>> If however gcc was actually wrong, then the code here is actually fine,
>> and a BUG_ON() or ASSERT() will never actually trigger.
>>
>> In addition, not a hotpath in the slightest, so performance isn't a concern.
>>
>>
>> I have still not managed to conclusively work out whether gcc is correct
>> or wrong. As a result, I would lean in the direction of BUG_ON() rather
>> than ASSERT(), out of paranoia. However, I would prefer even more a
>> solution where we were certain that gmfn isn't bogus.
>
> AFAICT GCC is wrong, though the code is confusing enough to me that I
> don't blame GCC for being confused too. :)
>
> I would be inclined to use a bigger hammer here. IMO refactoring like
> this makes it easier to reason about (compile tested only):
This looks like a pretty nice cleanup; I particularly like the 4 labels
going away.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
