Re: [PATCH v2 05/10] x86/HVM/SVM: Add AVIC initialization code

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 04/01/17 17:24, Suravee Suthikulpanit wrote:
> On 1/2/17 23:37, Andrew Cooper wrote:
>>> +
>>> +    vmcb->avic_bk_pg_pa = page_to_maddr(vlapic->regs_page) &
>>> AVIC_HPA_MASK;
>>
>> This use of AVIC_HPA_MASK may truncate the the address, which is
>> definitely a problem.
>
> I'm not quite sure that I got the truncation that you pointed out.
> Could you please elaborate?

My apologies.  I hadn't considered the PAGE_SHIFT in the definition, and
had come to the conclusion you were chopping the physical address off at
2^40.

>
>> If AVIC_HPA_MASK isn't just related to maxphysaddr, then you need to
>> pass appropriate memflags into the alloc_domheap_page() calls to get a
>> suitable page.
>>
>
> If by "maxphysaddr" is the 52-bit physical address limit for the PAE
> mode, then I think that's related.

You can safely rely on page_to_maddr() giving you a sensible value
without further masking.  By having a struct_page in the first place, it
is a known good frame.

>
>>> +
>>> +    entry = *(s->avic_last_phy_id);
>>> +    smp_rmb();
>>> +    entry.bk_pg_ptr = (vmcb->avic_bk_pg_pa & AVIC_HPA_MASK) >>
>>> AVIC_HPA_SHIFT;
>>> +    entry.is_running = 0;
>>> +    entry.valid = 1;
>>> +    *(s->avic_last_phy_id) = entry;
>>> +    smp_wmb();
>>
>> During domain creation, no guests are running, so you can edit this
>> cleanly.
> >
>> What values are actually being excluded here?  This, and other patches,
>> look like you are actually just trying to do a read_atomic(), modify,
>> write_atomic() update, rather than actually requiring ordering.
>>
>
> Besides the read_atomic(), modify write_atomic() to update the entry.
> I also want to make sure that the compiler won't shuffle the order
> around, which I thought can be achieved via smp_rmb() and smp_wmb().

Shuffle which order? Your smp_rmb() is between two writes, and the
smp_wmb() isn't paired with anything.

I think using read_atomic() and write_atomic() will DTRT for you; All
you appear to need is a guarantee that the code won't read/update the
live table using multiple accesses.

~Andrew

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