Re: [PATCH RFC V4 4/5] xen, libxc: Request page fault injection via libxc

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 08/05/2014 11:39 AM, Jan Beulich wrote:
>>>> On 05.08.14 at 10:09, <rcojocaru@bitdefender.com> wrote:
>> On 08/04/2014 06:20 PM, Jan Beulich wrote:
>>>>>> On 04.08.14 at 17:00, <rcojocaru@bitdefender.com> wrote:
>>>> On 08/04/2014 05:26 PM, Jan Beulich wrote:
>>>>>>>> On 04.08.14 at 13:30, <rcojocaru@bitdefender.com> wrote:
>>>>>> +    __vmread(VM_ENTRY_INTR_INFO, &ev);
>>>>>> +
>>>>>> +    if ( (ev & INTR_INFO_VALID_MASK) &&
>>>>>> +         hvm_event_needs_reinjection((ev >> 8) & 7, ev & 0xff) )
>>>>>
>>>>> Are there no manifest constants for all these plain numbers?
>>>>
>>>> If there are, vmx_vmcs_save() in vmx.c (line 416) doesn't use them. I've
>>>> copied that part verbatim.
>>>
>>> And that's precisely the problem: As long as there's exactly one use
>>> site, the need for manifest constants is questionable (i.e. largely
>>> cosmetic). As soon as there are multiple places, connecting them
>>> together is largely impossible without naming these numbers - only
>>> that way you have a reasonable chance to find the clone of the
>>> original should the original be found to need tweaking.
>>
>> I'll gladly add #defines for those magic constants, but could you please
>> recommend names for them and a header (or at least, category of headers)
>> to put them in, in the interest of minimizing the number of RFC versions
>> for this series?
> 
> Did you even make an attempt to locate a proper place, or
> to check whether such constants already exist? Simply looking
> for INTR_INFO_VALID_MASK would have turned up
> 
> #define INTR_INFO_VECTOR_MASK           0xff            /* 7:0 */
> #define INTR_INFO_INTR_TYPE_MASK        0x700           /* 10:8 */
> 
> i.e. all you need is there, you just need to make use of them
> (and please also - perhaps in an initial cleanup patch - for the
> original you cloned from). And just to avoid a needless further
> intermediate round: While there is no #define for the shift count
> used, you also don't need one if you make use of MASK_EXTR().

I did, obviously, but you can only turn up so much when grepping for "7"
in a large, mostly undocumented codebase. Thanks for your quick answer,
I'll use those and modify the original code as well.


Thanks,
Razvan Cojocaru