Re: [Xen-devel] [Patch v3] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.10.13 at 15:15, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>>> -    switch ( idx )
>>> +    switch ( idx - base )
>>>      {
>>> -    case 0:
>>> +    case 0: /* Write hypercall page */
>>>      {
>>>          void *hypercall_page;
>>> -        unsigned long gmfn = val >> 12;
>>> -        unsigned int idx  = val & 0xfff;
>>> +        unsigned long gmfn = val >> PAGE_SHIFT;
>>>          struct page_info *page;
>>>          p2m_type_t t;
>>>
>>> -        if ( idx > 0 )
>>> +        if ( val & 0xfff )
>> If you're not going to use 12, then shouldn't you use PAGE_SIZE-1 rather 
>> than 0xfff?
>>
>>>          {
>>> +            /* Bottom 12 bits are hypercall page index.  Only 0 is valid. 
>>> */
>> And modify this comment accordingly?
> 
> I suppose.  The use of ~PAGE_MASK before was my mistaken impression that
> this was an alignment check rather than a hypercall index check.
> 
> As this stuff is not documented anywhere I can find (other than by
> reading the code), I am not sure it is caring too much about.  Ideally,
> there would be somewhere XEN_MSR_HYPERCALL_PAGE_INDEX_MASK as 0xfff and
> XEN_MSR_HYPERCALL_PAGE_MAX_INDEX, but as it is unlikely that Xen will
> ever start using multiple hypercall pages, this sounds like overkill.
> 
> I am happy to implement it however people prefer, but would err on the
> lazy side of leaving it alone if there is no overwhelming objection.

As Paul said - if you use PAGE_SHIFT, you also should replace 0xfff.

Jan


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