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.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
