Re: [Xen-devel] [PATCH v2] SVM: Add union intstat_t for offset 68h in vmcb struct

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/03/2020 15:23, Pu Wen wrote:
> On 2020/3/25 18:30, Roger Pau Monné wrote:
>> On Tue, Mar 24, 2020 at 06:37:26PM +0800, Pu Wen wrote:
>>> diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h
>>> b/xen/include/asm-x86/hvm/svm/vmcb.h
>>> index b9e389d481..d8a3285752 100644
>>> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
>>> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
>>> @@ -316,6 +316,17 @@ typedef union
>>>       uint64_t raw;
>>>   } intinfo_t;
>>>   +typedef union
>>> +{
>>> +    struct
>>> +    {
>>> +        u64 intr_shadow:    1;
>>> +        u64 guest_intr_mask:1;
>>> +        u64 resvd:          62;
>>
>> Could you also use uint64_t for the fields, like you do below for
>> raw?
>
> Ok, thanks. Maybe bool for intr_shadow and guest_intr_mask is better?

Bool would be better if you're willing to change them.

There is a subtle truncation bug with can occur, e.g.

foo->intr_shadow = bar & MASK;

turns to 0 if MASK isn't the bottom bit, and intr_shadow is not bool.

The traditional way to fix this is with !!(bar & MASK), but bools are
safer because you can't get it wrong accidentally.

Its also fine to drop the resvd entirely.  No need for the field.

~Andrew