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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan


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