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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 05/19/2015 02:48 AM, Jan Beulich wrote:
>>>> On 18.05.15 at 18:12, <boris.ostrovsky@oracle.com> wrote:
>> On 05/18/2015 11:15 AM, Jan Beulich wrote:
>>>>>> On 08.05.15 at 23:06, <boris.ostrovsky@oracle.com> 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). 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).

-boris