Re: [Xen-devel] [PATCH v21 02/14] x86/VPMU: Add public xenpmu.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.05.15 at 16:40, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 05/19/2015 02:48 AM, Jan Beulich wrote:
>>>>> On 18.05.15 at 18:12, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 05/18/2015 11:15 AM, Jan Beulich wrote:
>>>>>>> On 08.05.15 at 23:06, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>> +/*
>>>>> + * Architecture-specific information describing state of the processor at
>>>>> + * the time of PMU interrupt.
>>>>> + * Fields of this structure marked as RW for guest can only be written 
>>>>> by the
>>>>> + * guest when PMU_CACHED bit in pmu_flags is set (which is done by the
>>>>> + * hypervisor during PMU interrupt). Hypervisor will read updated data in
>>>>> + * XENPMU_flush hypercall and clear PMU_CACHED bit.
>>>>> + */
>>>>> +struct xen_pmu_arch {
>>>>> +    union {
>>>>> +        /*
>>>>> +         * Processor's registers at the time of interrupt.
>>>>> +         * RW for hypervisor, RO for guests.
>>>>> +         */
>>>>> +        struct xen_pmu_regs regs;
>>>>> +        /* Padding for adding new registers to xen_pmu_regs in the 
>>>>> future */
>>>>> +#define XENPMU_REGS_PAD_SZ  64
>>>>> +        uint8_t pad[XENPMU_REGS_PAD_SZ];
>>>>> +    } r;
>>>>> +
>>>>> +    /* RW for hypervisor, RO for guest */
>>>>> +    uint64_t pmu_flags;
>>>>> +
>>>>> +    /*
>>>>> +     * APIC LVTPC register.
>>>>> +     * RW for both hypervisor and guest.
>>>>> +     * Only APIC_LVT_MASKED bit is loaded by the hypervisor into hardware
>>>>> +     * during XENPMU_flush.
>>>>> +     */
>>>>> +    union {
>>>>> +        uint32_t lapic_lvtpc;
>>>>> +        uint64_t pad;
>>>>> +    } l;
>>>>> +
>>>>> +    /*
>>>>> +     * Vendor-specific PMU registers.
>>>>> +     * RW for both hypervisor and guest.
>>>>> +     * Guest's updates to this field are verified and then loaded by the
>>>>> +     * hypervisor into hardware during XENPMU_flush
>>>>> +     */
>>>>> +    union {
>>>>> +        struct xen_pmu_amd_ctxt amd;
>>>>> +        struct xen_pmu_intel_ctxt intel;
>>>>> +
>>>>> +        /*
>>>>> +         * Padding for contexts (fixed parts only, does not include MSR 
>>>>> banks
>>>>> +         * that are specified by offsets)
>>>>> +         */
>>>>> +#define XENPMU_CTXT_PAD_SZ  128
>>>>> +        uint8_t pad[XENPMU_CTXT_PAD_SZ];
>>>>> +    } c;
>>>>> +};
>>>> Marking all the fields RW for the hypervisor is certainly correct from
>>>> a permissions pov, but requires close auditing that the hypervisor
>>>> doesn't ever read a field twice, potentially getting different results
>>>> and hence inconsistent internal state. Therefore - do all of the fields
>>>> _need_ to be RW for the hypervisor? If not, marking the ones
>>>> where this isn't needed as WO would be much preferred, to limit
>>>> the scope of whats needs to be audited.
>>> Right, all arch-independent bits are WO for hypervisor as are
>>> xen_pmu_regs above. I in fact meant to label them as such but for
>>> reasons that I can't remember now decided to mark them as RW.
>> Okay, that simplifies things for review purposes: Of the left
>> fields, lapic_lvtpc can easily be verified to be read just once, and
>> the vendor specific context gets copied into hypervisor memory
>> once before doing verification and loading. Which leaves pmu_flags:
>> Can you clarify how the read-write behavior there is expected to
>> be (perhaps by slightly extending the respective comment)? I ask
>> in particular because I don't recall having seen any read-once
>> enforcement in the code.
> 
> 
> pmu_flags are not read-once by neither hypervisor nor the guest. The 
> hypervisor uses PMU_CACHED flag (which it sets in the PMU interrupt) to 
> later determine whether or not to perform things like VPMU load (if the 
> flag is set then there is no reason to load). This may happen more than 
> once.
> 
> The rest of flags are WO by the hypervisor.
> 
> For the guest PMU_CACHED is indication that it shouldn't write MSRs 
> directly but rather do this into the shared page.
> 
> I don't believe that if guest writes flags (PMU_CACHED specifically) it 
> will have any effect on the hypervisor or other guests. It will 
> certainly affect the guest itself as it will probably mess up its VPMU 
> state. For example, we may end up loading the VPMU context when it 
> shouldn't be and that may result in an unexpected interrupt for the guest.
> I can easily add a hypervisor-private flag for this purpose (into 
> vpmu_struct.flags) to make things more cleanly delineated (i.e. 
> pmu_flags will be strictly WO by hypervisor and RO by the guest).

That would be good imo.

> I 
> still want to keep PMU_CACHED flag for the guest though so that it knows 
> where to write MSRs (Linux PV guests don't need this right now but other 
> guests may find this useful).

Understood.

Jan

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