Re: [Xen-devel] [V8 PATCH 7/8] pvh dom0: add check for pvh in vioapic_range

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.04.14 at 03:00, <mukesh.rathor@xxxxxxxxxx> wrote:
> On Mon, 7 Apr 2014 10:28:50 +0100
> George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> 
>> On 04/07/2014 07:59 AM, Jan Beulich wrote:
>> >>>> On 05.04.14 at 03:00, <mukesh.rathor@xxxxxxxxxx> wrote:
>> >> On Tue, 01 Apr 2014 16:09:15 +0100
>> >> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>> >>
>> >>>>>> On 01.04.14 at 16:40, <george.dunlap@xxxxxxxxxxxxx> wrote:
>> >>>> On 03/24/2014 09:34 AM, Jan Beulich wrote:
>> >>>>>>>> On 22.03.14 at 02:39, <mukesh.rathor@xxxxxxxxxx> wrote:
>> >>>>>> --- a/xen/arch/x86/hvm/vioapic.c
>> >>>>>> +++ b/xen/arch/x86/hvm/vioapic.c
>> >>>>>> @@ -238,8 +238,13 @@ static int vioapic_write(
>> >>>>>>
>> >>>>>>    static int vioapic_range(struct vcpu *v, unsigned long addr)
>> >>>>>>    {
>> >>>>>> -    struct hvm_hw_vioapic *vioapic =
>> >>>>>> domain_vioapic(v->domain);
>> >>>>>> +    struct hvm_hw_vioapic *vioapic;
>> >>>>>> +
>> >>>>>> +    /* pvh uses event channel callback */
>> >>>>>> +    if ( is_pvh_vcpu(v) )
>> >>>>>> +        return 0;
>> >>>>>>
>> >>>>>> +    vioapic = domain_vioapic(v->domain);
>> >>>>> I can see why the extra check is needed, but I can't see why you
>> >>>>> convert the initializer to an assignment: Afaict
>> >>>>> domain_vioapic() is safe even if d->arch.hvm_domain.vioapic ==
>> >>>>> NULL.
>> >>>> Or better yet, just make it something like:
>> >>>>
>> >>>> return vioapic && ((addr >= [...original range check]))
>> >>>>
>> >>>> That way we don't have to have a PVH-specific hook at all.  If a
>> >>>> domain doesn't have a vioapic for any reason, return 0.
>> >>> No, vioapic isn't going to be NULL for PVH:
>> >>>
>> >>> #define domain_vioapic(d)
>> >>> (&(d)->arch.hvm_domain.vioapic->hvm_hw_vioapic)
>> >> No, viopaic is NULL for PVH, hence the patch. So, can prob just
>> >> check for the ptr like George suggests and remove the pvh check.
>> > Sorry, no - I agree that d->arch.hvm_domain.vioapic is NULL for
>> > PVH, but that doesn't mean the result of domain_vioapic(d) is too
>> > (see the quoted #define above).
>> 
>> I interpreted Mukesh saying "check for the ptr" as, "check 
>> d->arch_hvm.domain.vioapic", not "check domain_vioapic(d)".
> 
> Correct. Since, vioapic is NULL for PVH, domain_vioapic(d) will
> result in NULL ptr exception for PVH,

Why/how? The macro doesn't deref that pointer, it only takes the
address of a field within the structure (which will result in a pointer
to or slightly above NULL).

Jan

> so we can't call that macro for PVH.
> 
> thanks
> Mukesh




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