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

[Wen Pu <puwen@xxxxxxxx>]

On 2020/3/26 0:03, Andrew Cooper wrote:
> 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.

Thanks for the explanation. I'm fine to use bool for intr_shadow and
drop the resvd field.

-- 
Regards,
Pu Wen